-
Notifications
You must be signed in to change notification settings - Fork 50
Fix dynamic wires with samples #1321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v0.44.0_rc
Are you sure you want to change the base?
Conversation
|
Hello. You may have forgotten to update the changelog!
|
This comment has been minimized.
This comment has been minimized.
dime10
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @rniczh !
pennylane_lightning/core/simulators/lightning_qubit/catalyst/LightningSimulator.cpp
Outdated
Show resolved
Hide resolved
| const std::vector<std::size_t> &wires, const std::string &kernelname, | ||
| std::size_t num_burnin, std::size_t num_samples) { | ||
| std::uniform_real_distribution<PrecisionT> distrib(0.0, 1.0); | ||
| std::size_t num_qubits = wires.size(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know that this is sufficient to identify which wires should be selected in a more complex dynamic (de)allocation scheme, or does it rely on the assumption that the selected wires are always the bottom n wires?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mentioned this offline but I think we'd be better off providing a universal solution that we never got around to implementing (see #1254 (comment)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow that was fast! 🤩
@maliasadi are you able to look this over?
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.44.0_rc #1321 +/- ##
==============================================
- Coverage 96.01% 93.89% -2.13%
==============================================
Files 307 243 -64
Lines 46923 40264 -6659
==============================================
- Hits 45055 37804 -7251
- Misses 1868 2460 +592
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
maliasadi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--------- Co-authored-by: Ali Asadi <[email protected]>
d3abf70 to
6e9f177
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work Hongsheng! Just a few questions and comments. Is there any reason we're implementing this for nvm, just saw Ali already addressed thislightning.qubit but not the other devices? Does this issue exist for the others as well?
pennylane_lightning/core/simulators/lightning_qubit/catalyst/LightningSimulator.cpp
Show resolved
Hide resolved
pennylane_lightning/core/simulators/lightning_qubit/catalyst/LightningSimulator.cpp
Show resolved
Hide resolved
pennylane_lightning/core/simulators/lightning_gpu/catalyst/LightningGPUSimulator.cpp
Outdated
Show resolved
Hide resolved
pennylane_lightning/core/simulators/lightning_gpu/catalyst/LightningGPUSimulator.cpp
Outdated
Show resolved
Hide resolved
| *(this->device_sv)}; | ||
| } | ||
|
|
||
| void LightningGPUSimulator::CompactStateVector() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are these tested? I couldn't find any tests for these methods for all devices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function is triggered by getMeasurements(), ensuring that any test executing the measurement process also updates the state vector. Currently, only one test case is added (the one that reported from the issue PennyLaneAI/catalyst#2339). The specific test can be found here:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this be added to Catalyst for the release?
per our offline discussions, what if these aux wires are entangled, we should test and add some handling mechanism for these cases in both Lightning & Catalyst?
pennylane_lightning/core/simulators/lightning_gpu/catalyst/LightningGPUSimulator.cpp
Outdated
Show resolved
Hide resolved
| auto GenerateSamples(size_t shots) -> std::vector<size_t>; | ||
|
|
||
| // Compact state vector by removing released qubits | ||
| void CompactStateVector(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: this is the style we've been following for a while:
| void CompactStateVector(); | |
| void compactStateVector(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we call this reducedStateVector instead of compact?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! I updated the name here: a5e2132
|
Could you also bump the rc version to 3 within |
Co-authored-by: Ali Asadi <[email protected]>
Co-authored-by: Ali Asadi <[email protected]>
Co-authored-by: Ali Asadi <[email protected]>
Sure abd373d |
jzaia18
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just 1 last comment
Context:
It get unexpected result due to the aux wires involved.
Description of the Change:
Only take the device wires for generating samplesCompact state vector before measurement.
For state vector normalization:
Benefits:
Possible Drawbacks:
Related GitHub Issues:
PennyLaneAI/catalyst#2339
[[sc-107206]]